repl: Enable tab completion for global properties when useGlobal is false #7369

lance · 2016-06-22T17:13:34Z

Checklist

make -j4 test (UNIX) or vcbuild test nosign (Windows) passes
a test and/or benchmark is included
the commit message follows commit guidelines

Affected core subsystem(s)

repl

Description of change

When useGlobal is false, tab completion in the repl does not enumerate
global properties. Instead of just setting these properties blindly on
the global context, e.g.

context[prop] = global[prop]

Use Object.defineProperty and the property descriptor found on
global for the new property in context.

Also addresses a previously unnoticed issue where console is writable
when useGlobal is false.

Ref: #7353

Fishrock123 · 2016-06-22T19:16:31Z

idea seems sound to me

addaleax · 2016-06-22T21:47:36Z

lib/repl.js

What does global in the inner context refer to now? I think it should still be context, right?

(The second line here also seems to have been superfluous?)

Both lines should stay the same

@addaleax - yes, this was an oversight...

princejwesley · 2016-06-22T23:21:05Z

@lance This solution has some side effects!

node 🙈 ₹ git:(master)  cat ~/Desktop/test.js
let repl = require('repl')
repl.start({useGlobal: false})
node 🙈 ₹ git:(master)  ./node !$
./node ~/Desktop/test.js
> [] instanceof Array
false
> /vm/ instanceof RegExp
false
>

lance · 2016-06-23T01:38:47Z

@princejwesley hmm - this is strange. I will take a look at this.

lance · 2016-06-23T20:44:04Z

@princejwesley this issue should be addressed with my latest push.
@addaleax your comments are addressed as well.

princejwesley · 2016-06-23T20:58:32Z

lib/repl.js

@lance instanceof test for this filtered list may fail!. e.g Int16Array. Can you confirm that? Adding those missing properties to GLOBAL_OBJECT_PROPERTIES will solve this issue.

I couldn't figure out the best way to test this programmatically, but the CLI behavior tests manually as expected.

~/s/node git:repl-useglobal-false-tab-completion ❯❯❯ ./node test-use-global-auto-complete.js > const x = new Int16Array() undefined > x instanceof Int16Array true >

I think this is the case because there is no builtin shorthand for Int16Array and the like. For builtin objects that have a shorthand representation such as [] or /\w+/ that representation is an instanceofthe builtin language object no matter what. But there is no builtin shorthand for Int16Array.

addaleax · 2016-06-23T23:54:22Z

nice work, LGTM with two questions

When `useGlobal` is false, tab completion in the repl does not enumerate global properties. Instead of just setting these properties blindly on the global context, e.g. context[prop] = global[prop] Use `Object.defineProperty` and the property descriptor found on `global` for the new property in `context`. Also addresses a previously unnoticed issue where `console` is writable when `useGlobal` is false. Ref: #7353

When `useGlobal` is false, all of the properties on `global` are copied to the new context (excluding `console` and `global`). Since `Object.getOwnPropertyNames()` returns all properties, not just enumerable ones, builtin properties on `global` are returned. We need to filter those out.

This existed in the original `repl.js` code, but it's not clear what the purpose is. It appears to be redundant.

jasnell · 2016-06-27T15:37:02Z

lib/repl.js

 const parentModule = module;
 const replMap = new WeakMap();

+const GLOBAL_OBJECT_PROPERTIES = ['NaN', 'Infinity', 'undefined',


I really wish we had a better way of tracking these... ah well.

jasnell · 2016-06-27T15:39:41Z

LGTM with a couple of nits

If the binary has been built with `./configure --without-intl` then the `Intl` builtin type will not be available in a repl runtime. Check for this in the test.

lance · 2016-06-27T19:07:47Z

CI: https://ci.nodejs.org/job/node-test-pull-request/3094/

@addaleax have I answered your questions well enough?
@jasnell nits addressed

addaleax · 2016-06-27T19:31:48Z

CI is green & this still LGTM

jasnell · 2016-06-27T20:14:00Z

LGTM

When `useGlobal` is false, tab completion in the repl does not enumerate global properties. Instead of just setting these properties blindly on the global context, e.g. context[prop] = global[prop] Use `Object.defineProperty` and the property descriptor found on `global` for the new property in `context`. Also addresses a previously unnoticed issue where `console` is writable when `useGlobal` is false. If the binary has been built with `./configure --without-intl` then the `Intl` builtin type will not be available in a repl runtime. Check for this in the test. Fixes: #7353 PR-URL: #7369 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

lance · 2016-06-27T21:51:39Z

Landed in: c0e48bf

When `useGlobal` is false, tab completion in the repl does not enumerate global properties. Instead of just setting these properties blindly on the global context, e.g. context[prop] = global[prop] Use `Object.defineProperty` and the property descriptor found on `global` for the new property in `context`. Also addresses a previously unnoticed issue where `console` is writable when `useGlobal` is false. If the binary has been built with `./configure --without-intl` then the `Intl` builtin type will not be available in a repl runtime. Check for this in the test. Fixes: #7353 PR-URL: #7369 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

MylesBorins · 2016-07-11T23:54:16Z

@lance @addaleax @nodejs/lts lts?

addaleax · 2016-07-12T00:03:20Z

Yes, but I think I’d like to give it a bit more time in v6? @lance feel free to overrule me here. :)

cjihrig · 2016-07-12T00:14:46Z

I agree, this should be in a couple v6 releases first.

lance · 2016-07-12T13:53:37Z

@addaleax @cjihrig wfm

When `useGlobal` is false, tab completion in the repl does not enumerate global properties. Instead of just setting these properties blindly on the global context, e.g. context[prop] = global[prop] Use `Object.defineProperty` and the property descriptor found on `global` for the new property in `context`. Also addresses a previously unnoticed issue where `console` is writable when `useGlobal` is false. If the binary has been built with `./configure --without-intl` then the `Intl` builtin type will not be available in a repl runtime. Check for this in the test. Fixes: #7353 PR-URL: #7369 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

This LTS release comes with 219 commits. This includes 80 commits that are docs related, 58 commits that are test related, 20 commits that are build / tool related, and 9 commits that are updates to dependencies. Notable Changes * build: - It is now possible to build the documentation from the release tarball (Anna Henningsen) #8413 * buffer: - Buffer will no longer incorrectly return a zero filled buffer when an encoding is passed (Teddy Katz) #9238 * deps: - upgrade npm in LTS to 2.15.11 (Kat Marchán) #8928 * repl: - Enable tab completion for global properties (Lance Ball) #7369 * url: - `url.format()` will now encode all `#` in `search` (Ilkka Myller) #8072 PR-URL: #9298

This LTS release comes with 219 commits. This includes 80 commits that are docs related, 58 commits that are test related, 20 commits that are build / tool related, and 9 commits that are updates to dependencies. Notable Changes * build: - It is now possible to build the documentation from the release tarball (Anna Henningsen) #8413 * buffer: - Buffer.alloc() will no longer incorrectly return a zero filled buffer when an encoding is passed (Teddy Katz) #9238 * deps: - upgrade npm in LTS to 2.15.11 (Kat Marchán) #8928 * repl: - Enable tab completion for global properties (Lance Ball) #7369 * url: - `url.format()` will now encode all `#` in `search` (Ilkka Myller) #8072 PR-URL: #9298

This LTS release comes with 219 commits. This includes 80 commits that are docs related, 58 commits that are test related, 20 commits that are build / tool related, and 9 commits that are updates to dependencies. Notable Changes * build: - It is now possible to build the documentation from the release tarball (Anna Henningsen) nodejs/node#8413 * buffer: - Buffer.alloc() will no longer incorrectly return a zero filled buffer when an encoding is passed (Teddy Katz) nodejs/node#9238 * deps: - upgrade npm in LTS to 2.15.11 (Kat Marchan) nodejs/node#8928 * repl: - Enable tab completion for global properties (Lance Ball) nodejs/node#7369 * url: - `url.format()` will now encode all `#` in `search` (Ilkka Myller) nodejs/node#8072 PR-URL: nodejs/node#9298 Signed-off-by: Ilkka Myller <[email protected]>

nodejs-github-bot added the repl Issues and PRs related to the REPL subsystem. label Jun 22, 2016

lance mentioned this pull request Jun 22, 2016

repl: Tab completion issue when useGlobal is set to false #7353

Closed

addaleax reviewed Jun 22, 2016
View reviewed changes

princejwesley reviewed Jun 23, 2016
View reviewed changes

lance added 4 commits June 27, 2016 10:39

repl: remove unused test code.

b4d8de7

repl: Remove context.global.global = context.

0991efe

This existed in the original `repl.js` code, but it's not clear what the purpose is. It appears to be redundant.

jasnell reviewed Jun 27, 2016
View reviewed changes

repl: Check Intl existence before test.

b1981b1

If the binary has been built with `./configure --without-intl` then the `Intl` builtin type will not be available in a repl runtime. Check for this in the test.

lance closed this Jun 27, 2016

Fishrock123 mentioned this pull request Jul 5, 2016

Propose v6.3.0 (v2) #7550

Merged

MylesBorins mentioned this pull request Jul 11, 2016

Audit commits not found on v4.x-staging #7661

Closed

MylesBorins added the lts-watch-v4.x label Jul 11, 2016

MylesBorins added land-on-v4.x and removed lts-watch-v4.x labels Oct 10, 2016

MylesBorins mentioned this pull request Oct 26, 2016

V4.6.2 proposal #9298

Closed

Uh oh!

repl: Enable tab completion for global properties when useGlobal is false #7369

repl: Enable tab completion for global properties when useGlobal is false #7369

Uh oh!

Conversation

lance commented Jun 22, 2016

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

Fishrock123 commented Jun 22, 2016

Uh oh!

addaleax Jun 22, 2016

Choose a reason for hiding this comment

Uh oh!

princejwesley Jun 22, 2016

Choose a reason for hiding this comment

Uh oh!

lance Jun 23, 2016

Choose a reason for hiding this comment

Uh oh!

princejwesley commented Jun 22, 2016

Uh oh!

lance commented Jun 23, 2016

Uh oh!

lance commented Jun 23, 2016

Uh oh!

princejwesley Jun 23, 2016

Choose a reason for hiding this comment

Uh oh!

lance Jun 23, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

addaleax commented Jun 23, 2016

Uh oh!

jasnell Jun 27, 2016

Choose a reason for hiding this comment

Uh oh!

jasnell commented Jun 27, 2016

Uh oh!

lance commented Jun 27, 2016

Uh oh!

addaleax commented Jun 27, 2016

Uh oh!

jasnell commented Jun 27, 2016

Uh oh!

lance commented Jun 27, 2016

Uh oh!

MylesBorins commented Jul 11, 2016

Uh oh!

addaleax commented Jul 12, 2016

Uh oh!

cjihrig commented Jul 12, 2016

Uh oh!

lance commented Jul 12, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

lance Jun 23, 2016 •

edited

Loading